Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @oshogbo)

packaging/docker/Dockerfile.test line 25 at r1 (raw file):

    meson setup \
        build/ \
	--prefix=/usr \

tabs -> spaces
plz fix your editor config ;)

packaging/docker/test.sh line 19 at r1 (raw file):

        codename="focal"
        ;;
    ubuntu22)

no 24.04?

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

packaging/docker/Dockerfile.test line 25 at r1 (raw file):

Half of it I developed on sdp12 and moved between machines to commit. On that machine, I don't have the right config.
NVM.
Sorry about that ;/

packaging/docker/test.sh line 19 at r1 (raw file):

These docker images are built based on the official Gramine packages (they do apt install from the official server).
As we don't have ubuntu24 images (AFIK) there is no way to test it.
When the new version is released (v1.8) I will add the support for ubuntu24 here and in the Docker build script.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)

-- commits line 7 at r2:
@oshogbo Could you add the Signed-off-by line for fixup commits also? I understand that it's useless, but GitHub's DCO checker fails because of this. And it's annoying to see a presumptive failure reported by GitHub...

packaging/docker/Dockerfile.test line 1 at r2 (raw file):

ARG GRAMINE_IMAGE=gramineproject/gramine:stable-focal

Is it reasonable to hard-code a particular version here? Wouldn't it be better to force the user to specify it explicitly (like it is done in the Bash script below)?

packaging/docker/Dockerfile.test line 13 at r2 (raw file):

    autoconf bison gawk git meson nasm libunwind-dev ninja-build pkg-config python3 \
    python3-click python3-jinja2 python3-pip python3-pyelftools python3-pytest \
    wget && \

This list will be annoying to modify. Can we split into one package per line (like we do in other cases, see e.g. CI dockerfiles)? Then it will be trivial to delete/add/sort.

packaging/docker/Dockerfile.test line 14 at r2 (raw file):

    python3-click python3-jinja2 python3-pip python3-pyelftools python3-pytest \
    wget && \
    python3 -m pip install 'meson>=0.56' 'tomli>=1.1.0' 'tomli-w>=0.4.0' && \

ditto (one package per line)

packaging/docker/Dockerfile.test line 25 at r2 (raw file):

    meson setup \
        build/ \
        --prefix=/usr \

Is there a specific reason why you choose the (non-default) /usr here? I mean, Ubuntu searches in both /usr and /usr/local (default in meson), so why not just use the default?

packaging/docker/Dockerfile.test line 27 at r2 (raw file):

        --prefix=/usr \
        -Ddirect=disabled \
        -Dsgx=disabled \

Doesn't it mean that this PR is blocked on #1913? Without that prerequisite PR, this meson compile will fail, isn't it?

packaging/docker/test.sh line 12 at r2 (raw file):

fi

image=""

Useless variable?

packaging/docker/test.sh line 38 at r2 (raw file):

    -t "${tag}" \
    -f Dockerfile.test \
    . || exit 1

Why this exit 1? Why not set -e?

packaging/docker/test.sh line 42 at r2 (raw file):

docker run \
    --rm \
    -ti \

I guess -t is needed to show PyTest output on the terminal, but do you really need -i? There's nothing interactive here.

packaging/docker/test.sh line 44 at r2 (raw file):

    -ti \
    --device /dev/sgx_enclave \
    --volume /var/run/aesmd/aesm.socket:/var/run/aesmd/aesm.socket \

You're testing gramine-direct (not SGX), so you don't need these two lines

packaging/docker/test.sh line 46 at r2 (raw file):

    --volume /var/run/aesmd/aesm.socket:/var/run/aesmd/aesm.socket \
    --cap-add SYS_ADMIN \
    --cap-add SYS_PTRACE \

These two capabilities should be explained in a comment. I guess ADMIN and PTRACE are for GDB tests?

Hm, but you don't specify these capabilities in the next docker-run (and that one also uses GDB tests), so ADMIN and PTRACE are required for something else...

packaging/docker/test.sh line 53 at r2 (raw file):

docker run \
    --rm \
    -ti \

ditto

Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)

packaging/docker/Dockerfile.test line 25 at r2 (raw file):
I think I found the reason, explained by @oshogbo himself:

If your --prefix differs from the actual installed gramine, then gramine-test won't be able to find libpal.

Taken from top-level discussion in #1913.

Reviewable status: 0 of 2 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

-- commits line 7 at r2:

Sure. I won't force-push it. I guess it was a comment for the feature.

packaging/docker/Dockerfile.test line 1 at r2 (raw file):

These Docker files are not really meant to be used manually I guess.
It also kinda documents what you are expecting, but I guess we can put a comment.
I have used the same scheme as in Dockerfile, so I don't really care if it's defined on not, I would just like to have the same in both files.

packaging/docker/Dockerfile.test line 13 at r2 (raw file):

Done.

packaging/docker/Dockerfile.test line 14 at r2 (raw file):

Done.

packaging/docker/Dockerfile.test line 25 at r2 (raw file):

Yes exactly.

packaging/docker/Dockerfile.test line 27 at r2 (raw file):

Yes, it will.
Althouth, I have mentioned an alternative way of testing this PR:

GRAMINE_URL=https://github.com/oshogbo/gramine-wips.git ./test.sh ubuntu20

But for an upstream, yes it will fail.

packaging/docker/test.sh line 12 at r2 (raw file):

Done.

packaging/docker/test.sh line 38 at r2 (raw file):

My idea was that you want to get the status of all test sgx and direct in one run.
Otherwise, you will end on the first run.

packaging/docker/test.sh line 42 at r2 (raw file):

Done.

packaging/docker/test.sh line 44 at r2 (raw file):

Done.

packaging/docker/test.sh line 46 at r2 (raw file):

Good catch, it seems that non of them are required any longer.

packaging/docker/test.sh line 53 at r2 (raw file):

Done.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)

-- commits line 7 at r2:

Yes, it's a comment for future.

packaging/docker/Dockerfile.test line 1 at r2 (raw file):

Ah, I see now. @oshogbo talks about this file: https://github.com/gramineproject/gramine/blob/master/packaging/docker/Dockerfile

Ok, resolving.

packaging/docker/Dockerfile.test line 25 at r2 (raw file):

Please add a comment before this RUN command about this

packaging/docker/Dockerfile.test line 27 at r2 (raw file):

I'll keep this blocking comment then, until #1913 is merged

packaging/docker/test.sh line 38 at r2 (raw file):

Good point, but can you add it as a top-level comment? Explaining that we don't want to set -e, because for actual runs of gramine-direct and gramine-sgx tests, we want to see them both (even if one of them fails).

packaging/docker/test.sh line 3 at r3 (raw file):

#!/usr/bin/env bash

Accidental empty line?